Skip to content

fix: refresh token utilization#997

Merged
sshanzel merged 10 commits intoWT-89-companion-feedbackfrom
WT-113-companion-token-issue
Jun 7, 2022
Merged

fix: refresh token utilization#997
sshanzel merged 10 commits intoWT-89-companion-feedbackfrom
WT-113-companion-token-issue

Conversation

@sshanzel
Copy link
Copy Markdown
Contributor

@sshanzel sshanzel commented Jun 6, 2022 •
edited
Loading

Changes

I think this is one comment from our beta user has really helped us finding the root cause.

As an FYI, I couldn’t commit the above message because of the “Access denied” error, but after a refresh of the page, I could. Same thing with the “markdown preview”, after the refresh it works. So perhaps a timing issue since I took a while to write the comment? Not sure, but I’ll keep an eye to see if I can reproduce it.

Describe what this PR does

  • We received quite a few feedback relating to making a comment. After further investigation, it was realized the token for making requests was not refreshed, meaning, every request gets rejected (upvotes/comments/upvotes modal/etc).
  • We are now utilizing the custom hook used in boot for refreshing the token.
  • Instead of re-requesting the whole boot data - we now instead request for the refreshed token.

Blocked until this PR is merged: https://github.com/dailydotdev/daily-gateway/pull/471

Please make sure existing components are not breaking/affected by this PR

Manual Testing

On those affected packages:

  • Have you done sanity checks in the webapp?
  • Have you done sanity checks in the extension?

Did you test the modified components media queries?

  • MobileL (420px)
  • Tablet (656px)
  • Laptop (1020px)

Did you test on actual mobile devices?

  • iOS (Chrome and Safari)
  • Android

Manually waited for quite some while to ensure the token is refreshed (vice-versa), to ensure workability.
Preview:
image

image

As it can be noticed, the requests have succeeded after the refresh token call.

Preview before the fix:
image

DD-WT-113 #done

@sshanzel
Copy link
Copy Markdown
Contributor Author

sshanzel commented Jun 6, 2022 •
edited
Loading

@rebelchris @idoshamun in time this gets approved. In my humble opinion, I think we should consider publishing to stores once merged as the actions themselves (upvotes/making a comment/etc) is what we expect from the users to accomplish.

@idoshamun
Copy link
Copy Markdown
Member

idoshamun commented Jun 6, 2022

The boot endpoint should already renew the access token when it's applicable. I suggest not introducing a new endpoint for refreshing a token at the moment. We can call boot if necessary again. I wonder why the token wasn't actually refreshed as we have everything in place. We need to find the root cause before introducing something new.

@sshanzel
Copy link
Copy Markdown
Contributor Author

sshanzel commented Jun 6, 2022 •
edited
Loading

I suggest not introducing a new endpoint for refreshing a token at the moment.

The reason why I created another one for refreshing the token, is the boot itself fetches a lot of data which are not needed in the situation.

I wonder why the token wasn't actually refreshed as we have everything in place. We need to find the root cause before introducing something new.

We instantiate the useRefreshToken hook in the BootProvider and we did not use that custom provider in the companion.

With that, we can definitely request from boot again if we don't mind the amount of data processed in that endpoint just to refresh the token.

@sshanzel
Copy link
Copy Markdown
Contributor Author

sshanzel commented Jun 6, 2022

There's a whole other permanent fix for this but I hesitated for the moment since we have the upcoming re-work of authentication.

The fix I was thinking of is, to create a middleware in the API server whenever we check the jwt validity and to refresh the token if found to be expired. So we won't have to create a timeout and force the refresh of token from the frontend.

@idoshamun
Copy link
Copy Markdown
Member

idoshamun commented Jun 6, 2022

to create a middleware in the API server whenever we check the jwt validity and to refresh the token if found to be expired. So we won't have to create a timeout and force the refresh of token from the frontend.

This is can introduce some security issues, so I wouldn't go down this path.

With that, we can definitely request from boot again if we don't mind the amount of data processed in that endpoint just to refresh the token.

I don't mind really. It will not happen that often. Thanks for considering it though :)
We might optimize this flow with the new authentication but for now let's keep it simple.

@sshanzel
Copy link
Copy Markdown
Contributor Author

sshanzel commented Jun 7, 2022 •
edited
Loading

Still works as intended. Attached Preview of the performed manual testing:

image

idoshamun
idoshamun approved these changes Jun 7, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@idoshamun idoshamun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great now! Please wait for Chris's review as well

rebelchris
rebelchris approved these changes Jun 7, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice this looks great!
Great find on abstracting the refresh token hook.

@sshanzel sshanzel merged commit ccca219 into WT-89-companion-feedback Jun 7, 2022
@sshanzel sshanzel deleted the WT-113-companion-token-issue branch June 7, 2022 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rebelchris rebelchris rebelchris approved these changes

@idoshamun idoshamun idoshamun approved these changes

@kirillkurko kirillkurko Awaiting requested review from kirillkurko

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sshanzel @idoshamun @rebelchris